Add Trainer port option for client-server strategies #198
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds an optional port parameter to the Trainer class that automatically configures the server port for client-server execution strategies. When provided, the port is forwarded to ClientServerExecutionStrategy instances, whether they are constructed by default, resolved from the registry, or provided as pre-built instances.
Key changes:
- Added
portparameter toTrainer.__init__()that forwards to client-server strategies - Updated
_make_strategy()to apply the port to default, registry-resolved, and pre-built strategies - Added comprehensive test coverage for port forwarding behavior with different strategy configurations
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| agentlightning/trainer/trainer.py | Added port parameter to Trainer class and updated strategy initialization to honor it |
| tests/trainer/test_trainer_init.py | Added three test cases covering port forwarding to client-server strategies and verifying it's ignored for non-client-server strategies |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if port is not None: | ||
| return ClientServerExecutionStrategy(n_runners=n_runners, server_port=port) | ||
| return ClientServerExecutionStrategy(n_runners=n_runners) | ||
|
|
There was a problem hiding this comment.
The conditional logic duplicates the ClientServerExecutionStrategy instantiation. Simplify by constructing kwargs conditionally and creating the strategy once.
| if port is not None: | |
| return ClientServerExecutionStrategy(n_runners=n_runners, server_port=port) | |
| return ClientServerExecutionStrategy(n_runners=n_runners) | |
| kwargs = {"n_runners": n_runners} | |
| if port is not None: | |
| kwargs["server_port"] = port | |
| return ClientServerExecutionStrategy(**kwargs) |
Summary
portargument toTrainerthat forwards toClientServerExecutionStrategyTesting
opentelemetrydependency in environment)https://chatgpt.com/codex/tasks/task_e_68f97fa44e1c832e9ac51733bbd00cf5